Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ApplicationTest : Increase pause before checking process name (2nd attempt) #6165

Merged
merged 1 commit into from
Jan 8, 2025
Merged

ApplicationTest : Increase pause before checking process name (2nd attempt) #6165

merged 1 commit into from
Jan 8, 2025

Conversation

ivanimanishi
Copy link
Member

This ensures the process has had time to change its own name.

This is part of the effort to get the gaffer tests to pass inside IE's environment.
1s wasn't enough of a pause to guarantee that the gaffer process had had time to startup, import the Gaffer module, and then rename the process, before the test would check for the name. That's likely related to the use of non-local storage and loading extra startups.

2s does seem to be enough, but I added an extra second for safety.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@ivanimanishi
Copy link
Member Author

@johnhaddon , other than resolving the conflict with the Changes file, I think this PR should be ok.
I initially targeted main, and I think that messed up the Version Check somehow.
But let me know if you think I need to address that, and how.

@ivanimanishi
Copy link
Member Author

@johnhaddon , anything I need to do on this PR?

@johnhaddon
Copy link
Member

@johnhaddon , anything I need to do on this PR?

Sorry for the delayed response. I was a bit hesitant to merge this as-is because Gaffer starts up fast in our environment, all these artificial little test delays do add up, and slow unit tests are bad for productivity. I don't suppose I could persuade you to do it with a loop that succeeds as soon as the name is correct, but errors if it is still wrong after 3s?

@ivanimanishi ivanimanishi force-pushed the moreForgivingProcessNameTest branch from daa7a4e to 5746640 Compare January 7, 2025 19:32
@ivanimanishi
Copy link
Member Author

Updated with the suggestion. Let me know what you think.
Note that in the last tests I ran locally, it actually finished before the 1s mark (though I don't know exactly why), so we may not strictly speaking need this PR anymore. But given that it's now taking less time than before in a faster host, it's probably a better implementation anyway.

@ivanimanishi @johnhaddon
ApplicationTest : More forgiving process name change test
9160952 
This ensures the process has had time to change its own name.
@johnhaddon johnhaddon force-pushed the moreForgivingProcessNameTest branch from aceb691 to 9160952 Compare January 8, 2025 14:34
@johnhaddon johnhaddon merged commit d5faceb into GafferHQ:1.5_maintenance Jan 8, 2025
5 checks passed
@johnhaddon
Copy link
Member

Thanks for going the extra mile on this one @ivanimanishi. I've rebased to fix the conflict and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnhaddon johnhaddon Awaiting requested review from johnhaddon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivanimanishi @johnhaddon